-
Notifications
You must be signed in to change notification settings - Fork 859
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
cleanup the code an try to make it faster #373
Conversation
Hi! Thanks for the patch! |
I have not done any real performance tests, so i wrote 'try'. |
Oh, I see.
Do you think we can totally replace
Honestly, code 'after' looks more complicated to me than 'before': (1) it has more conditions and nesting, (2) identical line that sets By the way, I've just checked
So typical
I suspect there's an easy perf win here: process |
That was my original idea but changing the type of the var will break serialization. |
Wait, I thought |
I simply did some timing tests with our ConsStringTest class.
Maybe the huge numbers are because of our unit tests (testAppendManyStrings). |
Most certainly. But even if I run only Thinking about this more, with |
ok made a new version available, at least here testAppendManyStrings() is 20% faster than the original version (when running with 50000000) |
Awesome! I've played around with some JMH tests (NOTE: I'm not a perf engineer, results could be wrong! I'll publish my tests later) and this implementation is noticeably faster:
🎉 There could possibly be a minor improvement if we switch to I also like the name change. What do you think about going with it slightly further: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks again for working on this!
I’ll be able to read this in detail next week.
We’re you able to use any of the benchmark suites already in there to see
if this made a difference? They’re very noisy and often don’t have
super-repeatable results but nonetheless the “v8benchmarks” target often
gives me a rough idea whether something has made things much worse — we
definitely don’t want to introduce anything that makes those worse!
…On Fri, Dec 29, 2017 at 4:42 AM Ivan Vyshnevskyi ***@***.***> wrote:
***@***.**** approved this pull request.
Thanks again for working on this!
—
You are receiving this because you are subscribed to this thread.
Reply to this email directly, view it on GitHub
<#373 (review)>,
or mute the thread
<https://github.com/notifications/unsubscribe-auth/AAf0a1y3cdwdrfdi1B5SXTzU212G63Woks5tFLQZgaJpZM4RNxJh>
.
|
I did 9 runs of v8 benchmark for code in master and with this patch. The median score I got is 43.6 (before) vs. 43.3 (after), which is comfortably inside error bounds, except maybe for Crypto. This result makes sense since neither of the test scripts seem to be heavy on String concatenation. Caliper in the SunSpider benchmark fails with “could not create the JVM”, does it works for you? (See my Google sheet with the data.) There's a new v8/web-tooling-benchmark, which supposed to have more real-life loads, but they require (at least) template strings support, so it doesn't run in Rhino right now. :( |
Thanks for all the detailed work. I'd like to get 1.7.8 out and since we did a "release candidate" I'd like to hold off on merging this until after that so that we minimize the changes to only bug fixes. Let me know if this is blocking something more urgent. Thanks! |
No problem for me, this is not critical.
RBRi
On Tue, 16 Jan 2018 09:25:14 -0800 Greg Brail wrote:
Thanks for all the detailed work. I'd like to get 1.7.8 out and since we did a "release candidate" I'd like to hold off on merging this until after that so that we minimize the changes to only bug fixes. Let me know if this
is blocking something more
…urgent. Thanks!
|
I decided this looked good too. Thanks! |
I decided this looked good too. Thanks!
Good decision ;-) Thanks for your support.
RBRi
--------------------------
WETATOR
Smart web application testing
http://www.wetator.org
|
No description provided.